Add --chown flag support for ADD/COPY commands for Windows #35521
Conversation
salah-khan
requested review from
cpuguy83,
dmcgowan,
dnephin,
lowenna,
johnstep and
tonistiigi
as code owners
salah-khan
changed the title
| SeRestorePrivilege = "SeRestorePrivilege" | ||
| SeBackupPrivilege = "SeBackupPrivilege" | ||
| SeRestorePrivilege = "SeRestorePrivilege" | ||
| SeTakeOwnershipPrivilege = "SeTakeOwnershipPrivilege" |
There was a problem hiding this comment.
Since winio is vendored, the change needs to be made in https://github.com/Microsoft/go-winio, merged there, released, and then vendored back here.
In the short term, define this constant elsewhere, such as in the system package, or in the file where it is used, perhaps with a TODO comment.
builder/dockerfile/copy_windows.go
Outdated
| func fixPermissions(source, destination string, rootIDs idtools.IDPair, overrideSkip bool) error { | ||
| // chown is not supported on Windows | ||
| func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error { | ||
|
|
There was a problem hiding this comment.
Nit: Starting a function with a blank line seems uncommon in this repository. I noticed other new functions that also start with a blank line, but will only comment on this once. :)
builder/dockerfile/copy_windows.go
Outdated
| // chown is not supported on Windows | ||
| func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error { | ||
|
|
||
| if identity.IdType == idtools.TypeIDSID { |
There was a problem hiding this comment.
Nit: Maybe invert the condition to avoid having to indent the rest of the function.
builder/dockerfile/copy_windows.go
Outdated
| // chown is not supported on Windows | ||
| func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error { | ||
|
|
||
| if identity.IdType == idtools.TypeIDSID { |
There was a problem hiding this comment.
Is it important to check the identity type here? It might be a good idea, but it seems like the same would apply to the Unix version, but checking for the other type. For LCOW, I recommend checking the target platform (OS), although I see LCOW does not support this feature yet, per the TODO comments in copy.go.
builder/dockerfile/copy_windows.go
Outdated
| func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error { | ||
|
|
||
| if identity.IdType == idtools.TypeIDSID { | ||
| var sid *windows.SID |
There was a problem hiding this comment.
Nit: Consider moving this down to the first use, with type inference (with :=):
sid, err := windows.StringToSid(identity.IdSid)| return "", false, err | ||
| } | ||
|
|
||
| if accountName == userStr { |
There was a problem hiding this comment.
Should this be a case-insensitive string comparison?
|
|
||
| accountSid = computerSid + "-" + strconv.FormatUint(uint64(userRid), 10) | ||
|
|
||
| defer windows.RegCloseKey(accountKey) |
There was a problem hiding this comment.
This defer should be moved up, right after opening the key.
| return "", false, err | ||
| } | ||
|
|
||
| if accountName == groupStr { |
There was a problem hiding this comment.
As with lookupNTUser, should this be a case-insensitive string comparison?
|
|
||
| accountSid = computerSid + "-" + strconv.FormatUint(uint64(groupRid), 10) | ||
|
|
||
| defer windows.RegCloseKey(accountKey) |
There was a problem hiding this comment.
As with logonNTUser, this defer should be moved up, right after opening the key.
| return accountSid, accountLocated, nil | ||
| } | ||
|
|
||
| func lookupNTGroup(groupNamesKey windows.Handle, groupStr string, computerSid string) (string, bool, error) { |
There was a problem hiding this comment.
This function is identical to lookupNTUser except for the variable names. Since it takes the key as a parameter, I'd eliminate this function entirely and rename the variables in lookupNTUser: maybe namesKey, nameStr, and rid. Perhaps rename the function to something like locateNTAccountFromKey.
| if accountName == "ContainerAdministrator" { | ||
| return idtools.Identity{IdType: idtools.TypeIDSID, IdSid: "S-1-5-93-2-1"}, nil | ||
|
|
||
| } else if accountName == "ContainerUser" { |
There was a problem hiding this comment.
These comparisons for ContainerAdministrator and ContainerUser should be case-insensitive.
builder/dockerfile/internals.go
Outdated
|
|
||
| if platform == "windows" { | ||
| return getAccountIdentity(chown, ctrRootPath) | ||
|
|
||
| } else { |
There was a problem hiding this comment.
This else is unnecessary since the if above always returns.
thaJeztah
added
status/2-code-review
area/builder
platform/windows
and removed
status/0-triage
labels
|
Looks like there's some linting issues; |
pkg/idtools/idtools.go
Outdated
| TypeIdentity | ||
| ) | ||
|
|
||
| type Identity struct { |
There was a problem hiding this comment.
Wondering if we should have a more simplified type;
- the type of identity to use depends on what platform/os the container is running on (correct?)
- theoretically, this means that both a Unix UID/GID and a Windows SID could be passed, and based on what platform it's executed on, the right type could be taken
Given the above, would something like below work?
type Identity struct {
UID int
GID int
SID string
}
|
In testing, I discovered that this only works for accounts in the SAM database of the base layer because the system registry hives are handled by kernel registry virtualization instead of the graph driver. Therefore, account lookup will fail for added users and groups. We are investigating possible solutions. |
builder/dockerfile/copy.go
Outdated
| @@ -55,7 +55,7 @@ func newCopyInfos(copyInfos ...copyInfo) []copyInfo { | |||
| return copyInfos | |||
| } | |||
|
|
|||
| // copyInstruction is a fully parsed COPY or ADD command that is passed to | |||
| // copyInstruction is a fully parsed COPY or ADD command that is `ed to | |||
pkg/idtools/idtools.go
Outdated
|
|
||
| func NewIDPairIdentity(IdPair IDPair) Identity { | ||
| return Identity{IdType: TypeIDPair, IdPair: IdPair} | ||
| } |
There was a problem hiding this comment.
This utility function seems fine, but I noticed most callers also initialize IdPair inline. Did you consider accepting UID and GID instead of IdPair, or adding yet another utility function?
salah-khan
requested review from
AkihiroSuda,
tianon and
vdemeester
as code owners
|
Please sign your commits following these rules: $ git clone -b "35507" git@github.com:salah-khan/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353944000
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
GordonTheTurtle
added
the
dco/no
GordonTheTurtle
removed
the
dco/no
|
This is ready; please review: @jhowardmsft @dnephin @tonistiigi @dmcgowan @vdemeester |
| } | ||
|
|
||
| if strings.EqualFold(accountName, "ContainerAdministrator") { | ||
| return idtools.Identity{SID: "S-1-5-93-2-1"}, nil |
There was a problem hiding this comment.
Those feels a bit like magic nubmers ? could we put them into constants and add a comment on it (for documentation purpose)
| return idtools.Identity{SID: "S-1-5-93-2-1"}, nil | ||
|
|
||
| } else if strings.EqualFold(accountName, "ContainerUser") { | ||
| return idtools.Identity{SID: "S-1-5-93-2-2"}, nil |
builder/dockerfile/copy_windows.go
Outdated
| cmd := reexec.Command("windows-fix-permissions", source, destination, identity.SID) | ||
| output := bytes.NewBuffer(nil) | ||
| cmd.Stdout = output | ||
| cmd.Stderr = output |
There was a problem hiding this comment.
@salah-khan could you make this change? I agree that the shorter code is more readable
builder/dockerfile/copy_windows.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func fixPermissionsReexec() { | ||
| err := fixPermissionsWindows(os.Args[1], os.Args[2], os.Args[3]) |
There was a problem hiding this comment.
Is there a need to check if len(os.Args) > 3)? Not sure if there is any chance that this would happen, but it would panic
builder/dockerfile/internals_unix.go
Outdated
|
|
||
| func getAccountIdentity(builder *Builder, accountName string, ctrRootPath string, state *dispatchState) (idtools.Identity, error) { | ||
| // This won't be called for non-Windows, but needs to be present since | ||
| // Windows has this function for obtaining NT account information. |
There was a problem hiding this comment.
👍 yes; if we don't need it, I'd prefer removing
| accountSid, err := sid.String() | ||
|
|
||
| if err != nil { | ||
| return idtools.Identity{SID: ""}, errors.Wrapf(err, "error converting SID to string") |
There was a problem hiding this comment.
Would it be useful to include sid and/or accountName as part of the error?
| if strings.EqualFold(accountName, "ContainerAdministrator") { | ||
| return idtools.Identity{SID: "S-1-5-93-2-1"}, nil | ||
|
|
||
| } else if strings.EqualFold(accountName, "ContainerUser") { |
There was a problem hiding this comment.
nit: no need to use else (first if already does a return
hack/make/containerutility
Outdated
| cd "$GOPATH/src/github.com/salah-khan/windows-container-utility" | ||
| git checkout -q "$CONTAINER_UTILITY_COMMIT" | ||
|
|
||
| echo Building: ${DEST}/containerutility.exe |
There was a problem hiding this comment.
@tiborvass @seemethere do you think this should use the docker- (future, possibly moby-) prefix? I know there's some scripts that (e.g.) cp docker-* or mv docker-* from the .zip we ship with binaries
There was a problem hiding this comment.
Yes it'd be useful for us when shipping Docker CE
There was a problem hiding this comment.
We don't use a docker- prefix on Docker running on Windows, so differentiating this from the current Windows model would make it confusing at best.
There was a problem hiding this comment.
The current executable for Windows is named dockerd.exe, not using that prefix. The Docker client is named docker.exe. I would rather use a list of files than a name prefix.
pkg/idtools/idtools_unix.go
Outdated
| // make an array containing the original path asked for, plus (for mkAll == true) | ||
| // all path components leading up to the complete path that don't exist before we MkdirAll | ||
| // so that we can chown all of them properly at the end. If chownExisting is false, we won't | ||
| // chown the full directory path if it exists | ||
|
|
||
| ownerUID := identity.UID | ||
| ownerGID := identity.GID |
There was a problem hiding this comment.
No need to use the intermediate variables here; it's cleaner to change the code below to
if err := lazyChown(pathComponent, identity.UID, identity.GID, nil); err != nil {Wonder if the argument should be renamed to owner instead?
| PROTECTED_DACL_SECURITY_INFORMATION = 0x80000000 | ||
| PROTECTED_SACL_SECURITY_INFORMATION = 0x40000000 | ||
| UNPROTECTED_DACL_SECURITY_INFORMATION = 0x20000000 | ||
| UNPROTECTED_SACL_SECURITY_INFORMATION = 0x10000000 |
There was a problem hiding this comment.
Go convention is to use CamelCase for constants as well; https://stackoverflow.com/a/22688926/1811501
Was there a specific reason to not use that here? If not, I think we should change to CamelCase
| SE_DS_OBJECT_ALL | ||
| SE_PROVIDER_DEFINED_OBJECT | ||
| SE_WMIGUID_OBJECT | ||
| SE_REGISTRY_WOW64_32KEY |
There was a problem hiding this comment.
This is exactly how these are named in Windows, so I kept them the same.
There was a problem hiding this comment.
Yes, they currently match the C constants, which is consistent with the Go syscall package.
Dockerfile
Outdated
| @@ -181,7 +181,6 @@ COPY hack/dockerfile/install/$INSTALL_BINARY_NAME.installer ./ | |||
| RUN PREFIX=/opt/$INSTALL_BINARY_NAME ./install.sh $INSTALL_BINARY_NAME | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
If you can revert to keep the diff focussed on the actual changes, that'd be appreciated 🤗
salah-khan
force-pushed
the
35507
branch
2 times, most recently
from
348735d
to
d25f9c5
Compare
|
@salah-khan I noticed there were still some outstanding review comments; were you working on those? |
salah-khan
force-pushed
the
35507
branch
2 times, most recently
from
de19896
to
12f49c7
Compare
salah-khan
force-pushed
the
35507
branch
6 times, most recently
from
2d2c501
to
1e39330
Compare
There was a problem hiding this comment.
It doesn't make sense to include the SID or account name as the error here is not specific to either. It's an internal error that occurred when converting a valid SID (which was obtained by converting a SID string) back into a SID string (for normalization - for example if the original sid was s-1-5-xyz-abc etc. it would be normalized to S-1-5-xyz-abc). But the specific SID or Account is not responsible for the error which is both likely to be exceedingly rare and only occur in low resource scenarios.
| source, _ := filepath.Split(os.Args[0]) | ||
|
|
||
| target := "C:\\Docker" | ||
| targetExecutable := target + "\\containerutility.exe" |
There was a problem hiding this comment.
This is already being built as part of the build process.
This implements chown support on Windows. Built-in accounts as well as accounts included in the SAM database of the container are supported. NOTE: IDPair is now named Identity and IDMappings is now named IdentityMapping. The following are valid examples: ADD --chown=Guest . <some directory> COPY --chown=Administrator . <some directory> COPY --chown=Guests . <some directory> COPY --chown=ContainerUser . <some directory> On Windows an owner is only granted the permission to read the security descriptor and read/write the discretionary access control list. This fix also grants read/write and execute permissions to the owner. Signed-off-by: Salahuddin Khan <salah@docker.com>
|
@thaJeztah @vdemeester Please take a look. All your feedback is addressed, and everything passes. |
|
|
||
| var daclPresent uint32 | ||
| var daclDefaulted uint32 | ||
| var dacl *byte |
There was a problem hiding this comment.
style nit; no need to change; generally we group these, i.e.;
var (
daclPresent uint32
daclDefaulted uint32
dacl *byte
)| } | ||
|
|
||
| func fixPermissionsReexec() { | ||
| err := fixPermissionsWindows(os.Args[1], os.Args[2], os.Args[3]) |
There was a problem hiding this comment.
nit: a more idiomatic way to write these is to scope the err to the if;
if err := fixPermissionsWindows(os.Args[1], os.Args[2], os.Args[3]); err != nil {
....
}| var daclDefaulted uint32 | ||
| var dacl *byte | ||
|
|
||
| err = system.GetSecurityDescriptorDacl(&securityDescriptor[0], &daclPresent, &dacl, &daclDefaulted) |
There was a problem hiding this comment.
same here (not a blocker)
if err := system.GetSecurityDescriptorDacl(&securityDescriptor[0], &daclPresent, &dacl, &daclDefaulted); err != nil {
return err
}| if strings.HasPrefix(accountName, "S-") || strings.HasPrefix(accountName, "s-") { | ||
| sid, err := windows.StringToSid(accountName) | ||
|
|
||
| if err == nil { |
There was a problem hiding this comment.
it's ok to ignore this error (if err != nil)?
There was a problem hiding this comment.
Yes, this is intentional. If an organization has accounts that use S- then falling through here will check on the actual system for the account (since it's not a valid SID). I know of at least one 50000+ employee organization with accounts of S-.
There was a problem hiding this comment.
Alright, thanks for checking!
| // Check if the account name is one unique to containers. | ||
| if strings.EqualFold(accountName, "ContainerAdministrator") { | ||
| return idtools.Identity{SID: system.ContainerAdministratorSidString}, nil | ||
|
|
| if strings.EqualFold(accountName, "ContainerAdministrator") { | ||
| return idtools.Identity{SID: system.ContainerAdministratorSidString}, nil | ||
|
|
||
| } else if strings.EqualFold(accountName, "ContainerUser") { |
There was a problem hiding this comment.
nit: no need for else here, because the if above already does a return
| @@ -18,8 +18,10 @@ func (daemon *Daemon) tarCopyOptions(container *container.Container, noOverwrite | |||
| return nil, err | |||
| } | |||
|
|
|||
| identity := idtools.Identity{UID: user.Uid, GID: user.Gid} | |||
There was a problem hiding this comment.
Wondering why you added the intermediate variable here, instead of putting it inline (as it was before)
| func (archiver *Archiver) IDMappings() *idtools.IDMappings { | ||
| return archiver.IDMappingsVar | ||
| // IdentityMapping returns the IdentityMapping of the archiver. | ||
| func (archiver *Archiver) IdentityMapping() *idtools.IdentityMapping { |
There was a problem hiding this comment.
Just leaving as note here; this is an exported method, and inside a pkg/, so will cause breaking changes for other projects that use this pkg (also for the other name/interface changes)
This is addressing: #35507
While the issue specifies warning that chown is not supported on
Windows, this fix actually implements chown support on Windows. Built-in
accounts as well as accounts included in the SAM database of the container
are supported.
The following are valid examples:
On Windows an owner is only granted the permission to read the security
descriptor and read/write the discretionary access control list. This
fix also grants read/write and execute permissions to the owner.
Signed-off-by: Salahuddin Khan salah@docker.com
- What I did
Added support for --chown on Windows which supports built-in accounts including ContainerAdministrator/ContatinerUser and accounts specific to the container. Examples of the following include \Administrator, \Guest and any specific users to the container.
- How I did it
This works by invoking another binary built from https://github.com/salah-khan/windows-container-utility within the container and extracts the SID information.
- How to verify it
The following Dockerfile sets both container specific and well known groups and uses ICACLS to verify that the user/group in question has the access requested.
The following is the output of building with the Dockerfile:
- Description for the changelog
Add --chown flag support for ADD/COPY commands for Windows
- A picture of a cute animal (not mandatory but encouraged)